Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[WIP] CVar rework #2552

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from
Open

Conversation

Rozelette
Copy link
Contributor

@Rozelette Rozelette commented Feb 28, 2023

This is a WIP example of reworking CVars to solve several issues with them. This is the SoH side of things, for the LUS PR, see Kenix3/libultraship#137.

The issues I am trying to fix:

  • CVar lookup is expensive since it involves a map lookup. It is one of the most expensive things that occurs in the game update loop (so outside of graphics/rendering)
  • CVar usage is error-prone. Since they are identified with strings, and defaulted if they don't exist (a common occurrence since they don't exist until they are changed), there is potential for misspellings to result in bugs that go undetected.

The changes needed in SoH are less structural, and more just needed as a client of the new interface. They do present an example of the intended interface. This PR reworks the values previously held in global.sav to be the new version of CVars. As such, they no longer need to be saved, since they will be saved with the CVars. Let's see what was needed to convert them.

  • There are new files, GlobalSettings.h/.cpp and z64settings.h
  • z64settings.h contains a global struct of settings that exist outside of any file-specific data. Data was moved out from gSaveContext accordingly.
  • GlobalSettings is the C++ side of things and manages defaulting and associating the data with CVars.

So, to add a new setting and associated CVar you:

  • Add data to the struct in z64settings.h
  • Add the default in GlobalSettings.cpp
  • Add the CVar also in GlobalSettings.cpp

That's it! Now you can use it just like a regular C variable and it'll get saved along with all the other CVars.

Update: made a second commit to show how to convert the existing usage into the new usage.

@Rozelette Rozelette added the do not merge Not ready or not valid changes label Feb 28, 2023
@@ -4971,7 +4972,7 @@ void Interface_Draw(PlayState* play) {
break;
}
} else {
if (CVarGetInteger("gCosmetics.Consumable_GreenRupee.Changed", rupeeWalletColors)) {
if (CVarGetInteger("gCosmetics.Consumable_GreenRupee.Changed", 0)) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As a note, this is a bug caught by the checks I added in the new CVars system. The default value was a pointer, instead of 0 (false) that it was everywhere else.

@dcvz
Copy link
Contributor

dcvz commented Mar 2, 2023

I've gone through this @Rozelette! I started here (and have not checked LUS) because when designing new systems I think its most important what it feels like to the user.

I really love the concept of everything just being vars that you can use in code and not having to worry about fetching and default values at the usage point.

One thing that I wonder if can be improved is that it's a 3 step process to make a new var. You add it to some struct, you register the default value for that property and then you register it as a Cvar (being persistable) if it needs to be persisted.

I wonder if there's any way we can achieve that with just one structure? Perhaps code generation? Perhaps a macro? Given that we're working in C-land that may not be possible but wanted to get gears turning.


It might not be possible but it would be great to be able to do something like:

// key that will be used, type, default value, persisted
CreateVar("global.language", s32, LANGUAGE_ENG, true);
CreateVar("global.zTargetSetting", s32, 0, false);

which maybe generates:

typedef struct {
   s32 language;
   s32 zTargetSetting;
} global;

global.language = LANGUAGE_ENG;
global.zTargetSetting = 0;

cvar->RegisterManaged("gLanguage", global.language);

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
do not merge Not ready or not valid changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants